Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix UG for CS2101 submission #559

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

RichDom2185
Copy link

No description provided.

@codecov-commenter
Copy link

Codecov Report

Merging #559 (e254679) into master (479610a) will decrease coverage by 0.20%.
The diff coverage is 45.45%.

@@             Coverage Diff              @@
##             master     #559      +/-   ##
============================================
- Coverage     65.25%   65.05%   -0.21%     
+ Complexity      737      736       -1     
============================================
  Files           147      147              
  Lines          2590     2598       +8     
  Branches        282      285       +3     
============================================
  Hits           1690     1690              
- Misses          749      753       +4     
- Partials        151      155       +4     
Impacted Files Coverage Δ
...main/java/seedu/foodrem/commons/core/Messages.java 0.00% <ø> (ø)
...seedu/foodrem/storage/JsonSerializableFoodRem.java 100.00% <ø> (ø)
...odrem/logic/commands/itemcommands/SortCommand.java 76.92% <33.33%> (-13.99%) ⬇️
.../logic/commands/itemcommands/FilterTagCommand.java 88.88% <50.00%> (-11.12%) ⬇️
...odrem/logic/commands/itemcommands/FindCommand.java 76.92% <50.00%> (-13.08%) ⬇️
src/main/java/seedu/foodrem/model/item/Item.java 51.13% <0.00%> (-2.28%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link

@bryanljx bryanljx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good though the fix for the broken link to the UG's layout section seems to break for DG.

To view more information, refer to the [Layout](/UserGuide.html#layout) section of the User Guide.
To view more information, refer to the [Layout](#layout) section of the User Guide.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line breaks for the DG, seeing how the pipeline is failing as well. This section is supposed to show in the DG and link to the UG's layout section so just [Layout](#layout) doesn't seem to be working for the DG tho it does fix the broken link for UG.

Alternatively, since the actual link we want is https://ay2223s1-cs2103t-w16-2.github.io/tp/UserGuide.html#layout while the previous link links to https://ay2223s1-cs2103t-w16-2.github.io/UserGuide.html#layout, maybe just adding the missing /tp like [Layout](/tp/UserGuide.html#layout) would suffice.

To view more information, refer to the [Layout](/UserGuide.html#layout) section of the User Guide.
To view more information, refer to the [Layout](#layout) section of the User Guide.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

To view more information, refer to the [Layout](/UserGuide.html#layout) section of the User Guide.
To view more information, refer to the [Layout](#layout) section of the User Guide.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this needs to be fixed, but this is not the fix. The issue is that if we don't use a fragment, this will break the PDF version of the UG since it will try to open a website instead. Working on a fix in the validation.

To view more information, refer to the [Layout](/UserGuide.html#layout) section of the User Guide.
To view more information, refer to the [Layout](#layout) section of the User Guide.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this needs to be fixed, but this is not the fix. The issue is that if we don't use a fragment, this will break the PDF version of the UG since it will try to open a website instead. Working on a fix in the validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants